Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use single ES document type #12794

Merged
merged 62 commits into from
Jul 19, 2017
Merged

Conversation

tylersmalley
Copy link
Contributor

@tylersmalley tylersmalley commented Jul 12, 2017

Technical Summary:

  • Removed compatibility with multi-type indices
  • ES Plugin status will be red when multiple, or zero, types are detected
    • devs will need to delete their kibana index or use the migration assistant UI to upgrade the index while still using 5.x
  • the ensureTypesExist part of the health check is now patchKibanaIndex, it no longer creates types and applies all patches in a single request
  • SavedObjectClient
    • Fixed #find() support for filtering on specific fields
    • Removed normalize_es_doc as the abstractions was subtly different for each method and seemed better implemented inline
    • fields and searchFields must be arrays after the API.
      • respective query params will always be cast to an array by using Joi.array().single()
  • server/mappings module added
    • includes what used to be ui/ui_mappings
    • exposes an instance of IndexMappings on kbnServer that should not be used by code outside of the uiExports
      • all mapping properties (except for the defaults) should be defined with uiExports type mappings in a plugin
    • defines server.getKibanaIndexMappingsDsl(), which was previously exposed via kibana.uiExports.mappings.getCombined()
      • code that used to pass around the mappings object and the server now just pass around the server
    • added helpers for reading mapping DSL
    • kbnServer.mappings which is the instance of IndexMappings maintained by the kibana server and extended by the UiExports class using kbnServer.mappings.addRootProperties()
  • when in the context of mappings management/es index types, SavedObject "types" are now disambiguated from es types by calling them the "root properties" of the "root type" of the "kibana index mappings dsl"
  • ui
    • SavedObjectLoader now sends * (not undefined*) when search string is undefined
    • Converted several uses of indexPattern.id to indexPattern.title, where appropriate
    • Added a mapBreadcrumbs() argument to routes that will allow them to set the display value for the index pattern id in the breadcrumb as the title of the index pattern
    • Removed a stray async function that snuck into angular code
    • When an index pattern doesn't have a title, the id is assumed to be the title (BWC)
  • functional tests
    • updated kibanaServer.uiSettings to use SavedObjectClient
    • added kibanaIndex service that fetches a copy of the kibanaIndex mapping and exposes the name of the kibana index to tests, should they need it
    • retry is now in the common services, along with kibanaServer and kibanaIndex

Release Note:

Starting in Elasticsearch 6.0, you are no longer able to create new indices with multiple index types. To accomplish the same effect as index types, a new type field was added to identify the document and the value mapped under the index to allow for strict mapping.

Example

"mappings": {
  "dashboard": {
    "properties": { ... }
  }
}

is now

"mappings": {
  "doc": {
    "properties": {
      "type": { "type": "keyword" },
      "dashboard": {
        "type": "object",
        "properties": { ... }
      }
    }
  }
}

Signed-off-by: Tyler Smalley <[email protected]>
@@ -19,7 +19,14 @@ class MappingsCollection {
}

getCombined = () => {
return this._currentMappings;
return {
'_default_': {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be necessary right? We can just add this to the doc type?

@tylersmalley
Copy link
Contributor Author

Jenkins, test it.

Tyler Smalley added 2 commits July 13, 2017 11:42
id,
type,
// handles documents with an _id of `${type}:${id}`
const docResponse = await this._withKibanaIndex('search', { body: createIdQuery({ type, id }) });
Copy link
Contributor Author

@tylersmalley tylersmalley Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a document is created in 5.x with multi-types, we create the document with the id provided.
When we re-index, we are pre-pending the type to the id due to uniqueness only being guaranteed per-type.
Currently, we are utilizing Elastisearch to provide the unique ID's and if we attempt to create a document with a specific ID, then we continue to prepend it.

I believe we should change using uuid.v1() or similar and auto-generate the ids ourselves. This would allow us to, beginning in 6.0, to do lookups/updates/gets by ID.

The change here fixes the issue and I am working to implement and test what I mentioned above.

Signed-off-by: Tyler Smalley <[email protected]>
@tylersmalley
Copy link
Contributor Author

Here is a PR I would like to pull into this branch: tylersmalley#2

@spalger @epixa, I added you to my fork so you're able to merge.

Tyler Smalley and others added 5 commits July 13, 2017 15:37
When creating an id, we will always prefix the type to either an id which was provided or one we generate. This alows us to simplify all actions for managing documents.

Signed-off-by: Tyler Smalley <[email protected]>
Remove entire compatibility layer and updates id generation
spalger
spalger previously approved these changes Jul 14, 2017
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but we probably want to extend the health check to go red when the index is using v5-format before merging

@stacey-gammon
Copy link
Contributor

stacey-gammon commented Jul 18, 2017

My little testing with dashboards and dashboard only mode, looks good to me, no issues! (Minus the index id thing showing up in the prefilled list of indexes, but looks like that might have been mentioned already above?)

@tylersmalley
Copy link
Contributor Author

@epixa - @spalger has a few more tests he is going to push up, then thoughts on merging to unblock on running against ES master? We will create issues for anything that comes up.

@tylersmalley
Copy link
Contributor Author

tylersmalley commented Jul 18, 2017

@spalger's changes LGTM

@epixa
Copy link
Contributor

epixa commented Jul 18, 2017

@tylersmalley I think that's a good idea.

@epixa
Copy link
Contributor

epixa commented Jul 18, 2017

We need to backport the necessary pieces as well. The longer they sit unbackported, the more likely we'll miss them and cause some really unfortunate regressions with the data, which are the hardest regressions to fix.

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jbudz
Copy link
Member

jbudz commented Jul 19, 2017

jenkins, test it. It failed on the view edit more test, #12014

@spalger
Copy link
Contributor

spalger commented Jul 19, 2017

Actually, failure seems legit,
dashboard app dashboard view edit mode panel expand control shown in edit mode 1

edit: @tylersmalley Looks like we made the wrong call, filtering the fields caused title^3 to be treated like an invalid field for dashboard, which is why that search failed to find anything^

@epixa
Copy link
Contributor

epixa commented Jul 19, 2017

Green!

@tylersmalley tylersmalley merged commit 3c0c0ff into elastic:master Jul 19, 2017
@tylersmalley
Copy link
Contributor Author

MERGED! cc: @elastic/kibana-operations

Working on a backport for the 5.x specific changes contained in this PR.

tylersmalley added a commit that referenced this pull request Jul 19, 2017
Partial backport containing 5.x specific changes
@tylersmalley
Copy link
Contributor Author

Partial backport to 5.x: cf5a044

@jimgoodwin jimgoodwin added Team:Operations Team label for Operations Team release_note:roadmap labels Jul 25, 2017
@jimgoodwin
Copy link

@tylersmalley can we get a Release Note: summary of the changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants